-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(in-cluster): do not allow the cluster to be used when disabled #21208
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Alexandre Gaudreault <[email protected]>
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21208 +/- ##
==========================================
- Coverage 55.17% 53.29% -1.89%
==========================================
Files 337 337
Lines 57057 57076 +19
==========================================
- Hits 31483 30416 -1067
- Misses 22870 24010 +1140
+ Partials 2704 2650 -54 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix the linting errors? It seems that there is a race condition being caught. The stack traces look like a similar problem I fixed before - https://github.com/argoproj/argo-cd/pull/20805/files
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
…sable Signed-off-by: Alexandre Gaudreault <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Thanks!
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
This reverts commit c96ecd8.
Signed-off-by: Alexandre Gaudreault <[email protected]>
…sable Signed-off-by: Alexandre Gaudreault <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes LGTM
@agaudreault can you fix the CI failures?
Current documentation seems to indicate that this setting is to disable the cluster. In every other cases, the behavior when a cluster is removed is for the apps to go in the unknown status and prevent sync from an invalid destination.
|
…sable Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
@ishitasequeira @terrytangyuan @rumstead lint/test fixed. Let's wait for discussion on the issue to know if we should cherry-pick. Thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm besides a few nitpicks. One final suggestion would be to change the tests to tabular tests for readability. Will go with your decision on whether that's worth the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
…sable Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Closes #21207
The ApplicationSet code is still using the in-cluster, even when disabled. However, the apps will fail in Argo CD, which is expected. See #10473
Depends on #21225
Existing applications:
New applications creation (API):